Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fp8 hipstream fix #3127

Closed

Conversation

acoskunses-AMD
Copy link
Contributor

Pick hip stream where cuda is not defined

Copy link

netlify bot commented Sep 12, 2024

Deploy Preview for pytorch-fbgemm-docs failed.

Name Link
🔨 Latest commit 1e4e129
🔍 Latest deploy log https://app.netlify.com/sites/pytorch-fbgemm-docs/deploys/6706fc278bdd20000858ec29

@acoskunses-AMD acoskunses-AMD force-pushed the fp8_hipstream_fix branch 2 times, most recently from 0c9fc22 to 863f596 Compare September 12, 2024 21:37
@acoskunses-AMD
Copy link
Contributor Author

@jianyuh @jwfromm

@xw285cornell
Copy link
Contributor

We use hipify script for this, is the change here needed?

@jianyuh
Copy link
Member

jianyuh commented Sep 19, 2024

Rebase to resolve conflicts: also as Xiaodong mentioned, internally we don't need this (automatically hipify). Is this needed on OSS workflow? I guess hipify torch is not enabled?

@@ -12,10 +12,13 @@
#include <numeric>

#include <ATen/ATen.h>
#if !defined(USE_ROCM)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hipify script should hipify .h right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in any case, I think maybe we can just remove this block just to be clean.

@@ -12,11 +12,14 @@
#include <numeric>

#include <ATen/ATen.h>
#include <c10/hip/HIPStream.h>
#if !defined(USE_ROCM)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this block even needed? If USE_ROCM is not defined, it'll basically be an empty file. So we can just remove this block here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just remove this include and move it down to the USE_ROCM. No need to include cuda header here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xw285cornell @jianyuh

changed as required. Please review.

@@ -12,10 +12,13 @@
#include <numeric>

#include <ATen/ATen.h>
#if !defined(USE_ROCM)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in any case, I think maybe we can just remove this block just to be clean.

@acoskunses-AMD acoskunses-AMD force-pushed the fp8_hipstream_fix branch 3 times, most recently from ee380a4 to 05e9b76 Compare September 30, 2024 21:39
@facebook-github-bot
Copy link
Contributor

@xw285cornell has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@xw285cornell xw285cornell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should never use nvcc to build those files so we should be ok?

@@ -201,4 +199,3 @@ at::Tensor f8f8bf16_rowwise_impl(
return Y;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acoskunses-AMD can you remove the blank lines at the end of each file? Otherwise we cannot land it internally due to some lint check

@facebook-github-bot
Copy link
Contributor

@xw285cornell has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@xw285cornell merged this pull request in 8e7beba.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants